Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1310 modify gasprices storage to support multi asset fees #1412

Merged

Conversation

Valentine1898
Copy link
Contributor

@Valentine1898 Valentine1898 commented Jul 3, 2024

Closes #1310
Fixes #1404
Fixes #1397

@Valentine1898 Valentine1898 linked an issue Jul 3, 2024 that may be closed by this pull request
Copy link

changeset-bot bot commented Jul 3, 2024

🦋 Changeset detected

Latest commit: 30d7049

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages
Name Type
@penumbra-zone/storage Major
@penumbra-zone/services Major
@penumbra-zone/query Major
@penumbra-zone/types Minor
@penumbra-zone/wasm Major
minifront Patch
node-status Patch
@penumbra-zone/crypto-web Major
@repo/ui Patch
@penumbra-zone/perspective Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Valentine1898 Valentine1898 force-pushed the 1310-modify-gasprices-storage-to-support-multi-asset-fees branch from 44c356a to 557677b Compare July 4, 2024 00:12
Copy link
Contributor

@TalDerei TalDerei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to properly implement alternative gas prices retrieved from the compact block!

tagging #1413 as relevant follow-up work.

Comment on lines +166 to +176
await this.indexedDb.saveGasPrices(
new GasPrices({
assetId: this.stakingAssetId,
...compactBlock.gasPrices,
}),
);
}
if (compactBlock.altGasPrices.length) {
for (const gasPrice of compactBlock.altGasPrices) {
await this.indexedDb.saveGasPrices(gasPrice);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this suggests that the RPC node is returning information in compact blocks about two alternative gas fees, but with zero gas prices. Manual testing confirmed that all other token denominations result in a "GasPrices not available" error.

Screenshot 2024-07-04 at 1 15 13 AM

return (
umNote.heightSpent === 0n &&
!isZero(getAmountFromRecord(umNote)) &&
umNote.addressIndex?.equals(addressIndex)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice filtering for address index here

Comment on lines -213 to -217
asset_id: alt_gas,
block_space_price: gas_prices.block_space_price * 10,
compact_block_space_price: gas_prices.compact_block_space_price * 10,
verification_price: gas_prices.verification_price * 10,
execution_price: gas_prices.execution_price * 10,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gas fee token price multiplier will instead be determined by governance proposals, rather than arbitrarily setting a 10x multiplier.

@Valentine1898 Valentine1898 merged commit 43ccd96 into main Jul 4, 2024
6 checks passed
@Valentine1898 Valentine1898 deleted the 1310-modify-gasprices-storage-to-support-multi-asset-fees branch July 4, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

more multi-asset fee bugs multi-asset fee bug Modify GasPrices storage to support multi-asset fees
2 participants